Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid dev packages when building the phar #37

Open
wants to merge 1 commit into
base: 1.x
Choose a base branch
from

Conversation

radford
Copy link
Contributor

@radford radford commented Jul 27, 2014

I'm not sure what "version" was being referred to when choosing installed.json over composer.lock, but the former has no notion of dev/no-dev whereas the later does. This works for me, but is a breaking change in that the current default includes dev packages. Alternatively there could be --dev / --no-dev options to build defaulting to --dev, but allowing --no-dev similar to composer install itself.

Presumably there should be --dev / --no-dev options in the future.
@clue
Copy link
Owner

clue commented Jun 6, 2015

I'm not sure what "version" was being referred to when choosing installed.json over composer.lock

Neither am I ;-) IIRC this was chosen in order to check which packages have actually been installed, vs which one have been locked.

IMHO it makes sense to skip pure require-dev packages either way.

As such, I'm all for getting this in! Can you update your PR with a few test cases to confirm this doesn't break anything?

This was referenced Jun 6, 2015
@clue
Copy link
Owner

clue commented Jun 18, 2015

I'm all for getting this in! Can you update your PR with a few test cases to confirm this doesn't break anything?

Ping @radford, is there anything I can help you with? :)

@clue
Copy link
Owner

clue commented Nov 7, 2015

Upon further inspection I think we should consider a few more things (just listing some random thoughts):

  • Running composer install --no-dev and then running phar-composer returns the expected phar with no dev dependencies installed (arguably a bit cumbersome, but explicit)
  • Running composer install (the default) and then running phar-composer returns a phar with dev dependencies installed (arguably not expected, but in line with how composer works)
  • The composer.lock is identical in both cases, whereas the installed.json only lists packages actually installed
  • Running phar-composer install already runs composer install --no-dev
  • If we install with dev dependencies but only pick non-dev dependencies, then the autoloader still contains dev dependencies (unlikely, but possible, to cause issues)
  • What if we run in a bare repository with only a composer.json (often the case with libraries)?
  • What if we run in a bare repository with both a composer.json and composer.lock (often the case with application projects)?

As an alternative approach, we may also look into merely detecting if dev dependencies have been installed. We could then either ask the user to confirm or abort or in the future also suggest installing into a temporary directory without dev dependencies.

Any thoughts?

@radford
Copy link
Contributor Author

radford commented Nov 8, 2015

Do you have a use case where dev dependencies in a phar make sense? To my mind, if you are releasing your dev dependencies, then they are not dev dependencies. I'd would suggest bumping the major version and dropping them.

@clue
Copy link
Owner

clue commented Nov 15, 2015

Do you have a use case where dev dependencies in a phar make sense?

Nope, afaict it never makes sense to bundle the dev dependencies and I'm all for dropping them! 👍

My previous comment was more about how we should drop them and what we have to keep in mind. Afaict some of these aren't addressed currently, so I'm not sure we should merge this yet.

What are your thoughts on this?

@radford
Copy link
Contributor Author

radford commented Nov 18, 2015

If we install with dev dependencies but only pick non-dev dependencies, then the autoloader still contains dev dependencies (unlikely, but possible, to cause issues)

This is a wart for sure, but I agree it is unlikely to cause problems.

What if we run in a bare repository with only a composer.json (often the case with libraries)?

I don't see the point of this use case. If the library has dependencies, then it should have a composer.lock to fix those versions.

What if we run in a bare repository with only a composer.json (often the case with libraries)?
What if we run in a bare repository with both a composer.json and composer.lock

If you are referring to a git bare repository, then I don't see how it makes sense to run in a bare repository. If by bare you mean in a repository where composer install hasn't been run, then I don't have an opinion.

Overall I think dropping dev dependencies is bug fix, but we should bump the version number in case there is someone that depends on the old behavior.

I suggest making the documented way to use phar-composer itself be as a dev dependency (which of course you wouldn't want to install) of any package that uses it for releases with its version number locked like as with your other dependencies.

@shivas
Copy link

shivas commented Nov 18, 2015

" If the library has dependencies, then it should have a composer.lock to fix those versions." - seriously? since when libraries have composer.lock? Libraries enforce "versions" with upper/lower bounds of versions in composer.json, and not the .lock.

@clue
Copy link
Owner

clue commented Jul 15, 2016

Thanks for filing this PR @Grisou13, I'd love to see this in! :shipit: Unfortunately, this PR is currently incomplete and hasn't seen any updates in some time.

I've just moved most of the discussion to #61, perhaps we should address that one first before continuing this PR? 👍

Would you guys care to comment on this ticket first? Thanks!

@radford
Copy link
Contributor Author

radford commented Jul 17, 2016

@clue, could you be more specific about what's missing from the PR? I stand by my contention that this is bug fix and the incompatibility should be handled by bumping the version number.

@clue
Copy link
Owner

clue commented Jul 18, 2016

I'd like to discuss this feature in #61, so that this PR only has to deal with its implementation details 👍

In particular two things are worthy of discussion (in #61):

  • What about the autoloader, because it contains entries for require-dev?
  • Should this be considered a BC break?

@schnittstabil
Copy link

Do you have a use case where dev dependencies in a phar make sense?

Personally, I'm fine with dropping them. However, if one needs to release a dev and a no-dev version, things get obviously complicated. E.g.:

# lean production version
$ php my.phar serve

# development version
$ php my_dev.phar serve --debug
$ php my_dev.phar run-tests         # internal unit tests on the current platform
$ php my_dev.phar create-docs docs/ # create html docs

@supun99x
Copy link

Any news on this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants